Use initial pen up value on cancel instead of an arbitrary value #163
Conversation
drskullster
force-pushed
the
fix-pen-height-on-cancel
branch
from
72b6ac5 to
7e517b8
Compare
| @@ -170,10 +170,8 @@ export async function startServer (port: number, hardware: Hardware = 'v3', com: | |||
| async executeMotion(motion: Motion, _progress: [number, number]): Promise<void> { | |||
| await ebb.executeMotion(motion); | |||
| }, | |||
| async postCancel(): Promise<void> { | |||
| const device = Device(ebb.hardware); | |||
There was a problem hiding this comment.
@drskullster did removing this line improve anything? Were there any side effects?
(I don't know its original purpose)
There was a problem hiding this comment.
@alexrudd2 from what I understand the Device object was necessary to get the machine value for a specific position (device.penPctToPos(50)). Since we now pass the position from the first pen motion on line 225 this is not needed anymore.
| @@ -189,7 +187,7 @@ export async function startServer (port: number, hardware: Hardware = 'v3', com: | |||
| console.log(`Motion ${progress[0] + 1}/${progress[1]}`); | |||
| await new Promise((resolve) => setTimeout(resolve, motion.duration() * 1000)); | |||
| }, | |||
| async postCancel(): Promise<void> { | |||
| async postCancel(_initialPenHeight: number): Promise<void> { | |||
There was a problem hiding this comment.
The 2 implementations should be kept in sync. (See #164 (comment)).
There was a problem hiding this comment.
@alexrudd2 so this is the simPlotter object that as far as I understand doesn't implement any action and just logs them. Interestingly the webserial implementation already checks for the pen up original position (or falls back to 50):
Line 185 in 03df63e
There was a problem hiding this comment.
Good point on the simPlotter.
Regarding the WebSerial implementation, that's probably the more robust implementation. The Math.max is screwy, but is actually intended to make things work with reversed output pins (where commanding the pen up actually moves it down),
Currently, when cancelling a plot the pen up value is arbitrarily set to 50. This PR makes it so it uses the initial pen up value (the same value that's used in the prePlot phase).